Support multiple artifact dependency aliases to the same package - #17067
Support multiple artifact dependency aliases to the same package#17067npmccallum wants to merge 7 commits into
Conversation
|
r? @weihanglo rustbot has assigned @weihanglo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@weihanglo are you able to review this? If not, maybe @ehuss could? |
|
@rustbot reroll |
|
Note that in general, there is a lot less review bandwidth at this time. Weihang has the most bandwidth and he said he wouldn't be available. We'll see when/if I have time. |
| authors = [] | ||
| resolver = "2" | ||
|
|
||
| [dependencies.bar] |
There was a problem hiding this comment.
Is this mimicking existing tests? I at least would prefer we only use inline tables for deps, not standard tables.
|
|
||
| p.cargo("build --unit-graph -Zunstable-options -Z bindeps") | ||
| .masquerade_as_nightly_cargo(&["unit-graph", "bindeps"]) | ||
| .with_stdout_contains(r#"[..]"index":0,"extern_crate_name":"bar"[..]"#) |
There was a problem hiding this comment.
Is this mimicking an existing pattern? We try to avoid the _contains functions
This comment has been minimized.
This comment has been minimized.
|
I split this into smaller commits:
I also adjusted the tests from the inline comments to use inline dependency tables and a structured unit-graph stdout assertion. |
|
May have time for this, though let me pick up the RFC first. |
4546761 to
55cb078
Compare
This comment has been minimized.
This comment has been minimized.
|
@joshtriplett This is my next highest priority if you have cycles. With this feature, arma can generate PMIs cross-architecture. |
This comment has been minimized.
This comment has been minimized.
| .unwrap_or_else(|| (to_target.crate_name().into(), None)) | ||
| } | ||
|
|
||
| pub fn lib_dependency_name<'a>( |
There was a problem hiding this comment.
This appears to be dead code within this commit
| Ok((extern_crate_name, dep_name)) | ||
| } | ||
|
|
||
| pub fn dep_extern_crate_name_and_dep_name( |
There was a problem hiding this comment.
Doesn't look like this needs to be pub within this commit
| [build-dependencies.bar] | ||
| path = "bar/" | ||
| artifact = "bin" | ||
| target = "{}" | ||
|
|
||
| [build-dependencies.bar-native] | ||
| package = "bar" | ||
| path = "bar/" | ||
| artifact = "bin" | ||
| target = "{}" | ||
| [build-dependencies] | ||
| bar = {{ path = "bar/", artifact = "bin", target = "{}" }} | ||
| bar-native = {{ package = "bar", path = "bar/", artifact = "bin", target = "{}" }} |
There was a problem hiding this comment.
Was this style change made in the wrong commit? It makes it harder to see what may be intentional changes fthat go with this fix
| .with_status(101) | ||
| .with_stderr_data(str![[r#" | ||
| [LOCKING] 1 package to latest compatible version | ||
| [ERROR] the crate `foo v0.0.0 ([ROOT]/foo)` depends on crate `bar v0.5.0 ([ROOT]/foo/bar)` multiple times with different names | ||
|
|
||
| "#]]) | ||
| .with_stderr_contains( | ||
| "[RUNNING] `rustc --crate-name bar --edition=2015 bar/src/main.rs [..]--target [ALT_TARGET] [..]", | ||
| ) |
There was a problem hiding this comment.
What is the goal with using with_stderr_contains with this line of verbose output?
For context, we try to minimize the use of _contains assertions because we can't do snapshotting with them
| #[cargo_test] | ||
| fn multiple_artifact_aliases_no_op_rebuild() { |
There was a problem hiding this comment.
Why is a new test being added in a non-test commit?
| .with_stdout_contains(r#"[..]"rename":"bar-lib"[..]"#) | ||
| .with_stdout_contains(r#"[..]"rename":"bar-bin"[..]"#) | ||
| .with_stdout_contains(r#"[..]"extern_name":"bar_lib"[..]"#) | ||
| .with_stdout_contains(r#"[..]"extern_name":"bar_bin"[..]"#) |
There was a problem hiding this comment.
The lack of context is another problem with _contains assertions: I was looking to the test to better understand a part of your change and I can't tell what is happening from this
Record the duplicate-name diagnostics produced when one package is requested through multiple artifact aliases. Cover build and normal dependencies, binary and static-library artifacts, same- and cross-target aliases, and the `--unit-graph` entry point. The snapshots capture how dependency-name validation collapses distinct artifact requests onto one package edge.
Record how `cargo metadata` resolves multiple declarations for one package. A single artifact alias retains the package-edge name, while an artifact and an ordinary dependency to a bin-only package can coexist because there is no library edge to collide. Complete JSON snapshots keep manifest declarations, package-level resolve edges, and dep-kind extern names visible together. Rejection coverage captures multiple artifact aliases and mixed artifact and ordinary dependencies when the package provides a library.
Move the per-dependency extern-crate and manifest-name calculation into a private helper while preserving the existing duplicate-name validation and behavior.
Treat each artifact dependency declaration as a distinct dependency identity when constructing units, resolving features, and emitting metadata. This allows one package to supply artifacts under multiple aliases or for multiple targets without collapsing their names or feature sets. An artifact dependency no longer doubles as a Rust library dependency. Reject the dependency-level `lib` key; users that need both forms can declare a normal dependency and an artifact dependency. Keep those declarations separate so artifact features do not unify with each other or with the library dependency. Carry alias-specific names through artifact units, unit graphs, Cargo tree nodes, environment variables, and metadata. Normal dependency names continue to determine library edges, while artifact dep-kind entries report each alias.
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
This update expands the scope of the PR in one deliberate way: dependency-level While implementing independent artifact aliases, it became clear that retaining it would leave the library edge ambiguously attached to one of several aliases, targets, and feature sets—or would require unifying declarations that must remain independent. The revised design requires an explicit normal dependency when the library is also needed. I have made this design change explicit in the implementation, tests, documentation, commit message, and updated PR description rather than treating it as an incidental consequence. The rewritten five-commit series separates characterization tests, metadata tests, the resolver refactor, the behavior change, and documentation. |
| p.cargo("metadata --format-version 1 -Z bindeps") | ||
| .masquerade_as_nightly_cargo(&["bindeps"]) | ||
| .with_stdout_data(str![[r#" | ||
| {"packages":[{"name":"bar","version":"0.5.0","id":"path+[ROOTURL]/foo/bar#0.5.0","license":null,"license_file":null,"description":null,"source":null,"dependencies":[],"targets":[{"kind":["lib"],"crate_types":["lib"],"name":"bar","src_path":"[ROOT]/foo/bar/src/lib.rs","edition":"2015","doc":true,"doctest":true,"test":true},{"kind":["bin"],"crate_types":["bin"],"name":"bar","src_path":"[ROOT]/foo/bar/src/main.rs","edition":"2015","doc":true,"doctest":false,"test":true}],"features":{},"manifest_path":"[ROOT]/foo/bar/Cargo.toml","metadata":null,"publish":null,"authors":[],"categories":[],"keywords":[],"readme":null,"repository":null,"homepage":null,"documentation":null,"edition":"2015","links":null,"default_run":null,"rust_version":null},{"name":"foo","version":"0.5.0","id":"path+[ROOTURL]/foo#0.5.0","license":null,"license_file":null,"description":null,"source":null,"dependencies":[{"name":"bar","source":null,"req":"*","kind":null,"rename":"bar-alias","optional":false,"uses_default_features":true,"features":[],"artifact":{"kinds":["bin"],"lib":false,"target":null},"target":null,"registry":null,"path":"[ROOT]/foo/bar"}],"targets":[{"kind":["lib"],"crate_types":["lib"],"name":"foo","src_path":"[ROOT]/foo/src/lib.rs","edition":"2015","doc":true,"doctest":true,"test":true}],"features":{},"manifest_path":"[ROOT]/foo/Cargo.toml","metadata":null,"publish":null,"authors":[],"categories":[],"keywords":[],"readme":null,"repository":null,"homepage":null,"documentation":null,"edition":"2015","links":null,"default_run":null,"rust_version":null}],"workspace_members":["path+[ROOTURL]/foo#0.5.0"],"workspace_default_members":["path+[ROOTURL]/foo#0.5.0"],"resolve":{"nodes":[{"id":"path+[ROOTURL]/foo/bar#0.5.0","dependencies":[],"deps":[],"features":[]},{"id":"path+[ROOTURL]/foo#0.5.0","dependencies":["path+[ROOTURL]/foo/bar#0.5.0"],"deps":[{"name":"bar_alias","pkg":"path+[ROOTURL]/foo/bar#0.5.0","dep_kinds":[{"kind":null,"target":null,"extern_name":"bar_alias","artifact":"bin","bin_name":"bar"}]}],"features":[]}],"root":"path+[ROOTURL]/foo#0.5.0"},"target_directory":"[ROOT]/foo/target","build_directory":"[ROOT]/foo/target","version":1,"workspace_root":"[ROOT]/foo","metadata":null} |
There was a problem hiding this comment.
There a reason this isn't being done as a json snapshot like our other metadata tests?
I have been wondering about simplifying that down. I have also wondered about going a step further and only allowing one artifact per dependency specification. This seems like a big design change and it feels buried within a commit for allowing multiple edges to the same package. Is there a way t we can split that out into its own PR, even if there are some hacky things along the way that would then get cleaned up? And does this mean we should remove |
Artifact dependencies no longer also act as Rust library dependencies. Remove `lib` from the dependency manifest schema and artifact metadata. Retain the legacy registry fields solely for wire compatibility. Ignore the flag when reading index entries, record `false` in generated index entries, and omit it from publish uploads. Report a targeted migration error when a manifest uses the removed key.
|
Thanks—you’re right that this is a design change and that it needs to be explicit. However, I believe that this PR represents the only reasonable design. Each bindep declaration defines a distinct dependency tree: its identity, compile target, and feature selection. Its artifact list only selects which outputs Cargo builds from that tree.
If “one artifact per dependency specification” means one declaration per binary, I think that puts the boundary in the wrong place. Multiple outputs can legitimately share one dependency tree and feature set, which is exactly what an artifact list expresses. Conversely, outputs needing different features can use the same package under separate aliases with disjoint artifact lists: [build-dependencies.tools-stable]
package = "tools"
artifact = ["bin:fmt", "bin:lint"]
features = ["stable"]
[build-dependencies.tools-experimental]
package = "tools"
artifact = ["bin:pack", "bin:ship"]
features = ["experimental"]That is why the removal belongs in this PR. I’ve made the complete surface removal explicit in its own commit, covering the manifest schema, artifact model, and metadata; the historical registry field remains only for compatibility and is ignored. Moving this to another PR would leave Cargo in a knowingly incoherent intermediate state. |
Removing the dependency-level `lib` field is a breaking change to the public manifest schema API. Advance the pre-1.0 minor version accordingly.
Show how separate aliases request artifacts from the same package for multiple target platforms. Replace the removed dependency-level `lib` option with explicit normal and artifact dependency declarations. Explain that their feature graphs remain independent and show how to migrate existing manifests.
For context, there is some regret on even allowing multiple addressable crates within one package. I also wonder how much using multiple bins will be done in practice.
Within the same
I appreciate it being its own commit. What isn't too clear is why this can only be done where it is an incoherent state? Another option would be to remove it before this PR though that would require also updating any tests that set both a lib and an artifact. Might be better this way. |
View all comments
This fills in behavior described by RFC 3176 that was not implemented when artifact dependencies were initially added.
The RFC allows a package to be declared multiple times under different dependency names, with each declaration selecting a different artifact target. This PR treats each artifact dependency declaration as an independent identity across unit construction, feature resolution, Cargo tree output, environment variables, unit graphs, and metadata. This allows one package to supply artifacts under multiple aliases or for multiple targets without collapsing their names or feature sets.
Design change: dependency-level
libThis expands the original scope by removing the dependency-level
liboption. Preservinglib = trueis not coherent once one package may appear through multiple artifact declarations: those declarations can have different aliases, targets, and feature selections, while the package library edge has only one dependency identity. Selecting one artifact declaration as the library dependency would be ambiguous, and combining them would collapse the independent feature graphs that this change requires.Users that need both the library and an artifact must declare a normal dependency and a separately named artifact dependency. Those declarations retain independent feature graphs. Cargo rejects the dependency-level
libkey whether it istrueorfalse. This is a deliberate design change to the unstable artifact-dependencies feature. The historical registry-index field is retained for compatibility but no longer makes an artifact declaration double as a library dependency.The series is split into reviewable commits:
libkey.The rebased series was reviewed as a whole and every commit independently passed formatting, the Cargo build and test suite, the remaining workspace tests, and resolver-tests.
Closes #12374
FYI @joshtriplett @Byron